Polar integration #461
Polar integration #461
Conversation
- improve encapsulation and separation of concerns - simplify stats calculation
- add JSDoc comments for PaymentProcessor methods - improve JSDoc comments for exported object
- Enable processor selection by environment variable or specified override for testing scenarios - Refactor env var validation
- Add type-safety to webhook handler - Apply current Wasp env var validation
|
@vincanger Still a WIP, but as I've gotten to the point where I can create orders and subscriptions, I thought I should share the changes I've made that affect other parts of the codebase for discussion. Configurable provider selectionThe current codebase requires the developer implementing the template to modify the codebase to select which payment provider to implement. I understand the reasoning behind this decision, but it also makes it impossible to implement e2e tests for multiple providers without modifying the codebase. To address this, I implemented a new Updated schema validationOpenSaas currently uses a custom validation function to ensure the required env vars are set, but with the introduction of Zod validation, this seems redundant and so I implemented Zod-based validation for this provider, which could also be applied to the existing platforms, if desired. Refactored stats jobThe stats job previously contained functions for both Stripe and LemonSqueezy, which felt like a bit of code smell as it violates the open/closed principle, so I refactored that code to make the revenue calculation a function of the PaymentProcessor interface to be implemented by each integration. |
There was a problem hiding this comment.
Did a quick review. I see out moduleResolution is causing import issues for the polar SDK. Hopefully we can get that sorted out quick. Everything looks to be on the right track though! The main thing I'd like to change at the moment is to remove comments that are redundant as many of them just repeat what's discernible from the function name.
| */ | ||
|
|
||
| // @ts-ignore | ||
| import { WebhookBenefitCreatedPayload } from '@polar-sh/sdk/models/components/webhookbenefitcreatedpayload.js'; |
There was a problem hiding this comment.
Ok so we just opened an issue to fix the moduleResolution option on the Wasp end so that we can avioid this issue. So hopefully that gets done ASAP and we can merge the PR without it...
There was a problem hiding this comment.
I would wait on merging this maybe before moduleResolution fix. Hopefully, we do get it before this is finished.
- Fix order status checking - Remove redundant subscription status mapping type and custom status values - Remove redundant JSDoc comments
There was a problem hiding this comment.
Hey @Genyus,
went over the PR.
Great work on this.
I'm sorry that we are asking you to remove so much from PR, but we want to focus on getting only Polar working here first.
Also I would appreciate it if you could reduce the amount of unnecessary jsdocs/comments. Didn't want to point it out everywhere since there is a lot. Do take care to use it only when we need it.
For now I've only went through the code, didn't run anything yet.
After you implement changes I would like to actually test everything out with Polar sandbox account. I would recommend you do the same.
| @@ -3,6 +3,9 @@ | |||
| # If you use `wasp start db` then you DO NOT need to add a DATABASE_URL env variable here. | |||
| # DATABASE_URL= | |||
|
|
|||
| # Supports Stripe, LemonSqueezy, Polar | |||
| PAYMENT_PROCESSOR_ID=Stripe | |||
There was a problem hiding this comment.
Hey, it's great to see that you're trying to improve open-saas further above the Polar integration, but I wouldn't do this as part of this PR.
Paddle integration PR had the same problem here:
https://github.com/wasp-lang/open-saas/pull/486/files#r2266908329
Doing in this PR will distract us from the main point (Polar), and will prolong the process to get the feature we want.
I've explained in the Paddle PR (linked above) why we don't believe this approach is right for us.
I would kindly ask you to remove non-Polar parts of the PR.
Thanks for the effort.
There was a problem hiding this comment.
@FranjoMindek Thanks for the feedback. Happy to proceed, but one of the main reasons I adopted this approach was because I wanted to add e2e tests for Polar and the current architecture makes it impossible to run tests for more than one provider without modifying the source. Do you have any suggestions for how that limitation could be addressed?
There was a problem hiding this comment.
Hey @Genyus.
Truthfully, e2e-tests are in a bad state and outdated.
We will want to improve them but that is a separate issue.
However we did start opening up issues about them:
#469
I wouldn't be concerned with improving them in this PR.
I think it's okay for now to require people to change the app to test different payment providers in e2e-tests.
What we want of e2e-tests is that we have separate package.json scripts to run for different providers.
So if we have the correct payment provider in app and run the correct script, it works.
So I would rename the current scripts to be more explicit about stripe and then add scripts for Polar.
This is kinda harder to do fully correct because we would have to change a lot of script names.
I would most likely suggest to do minimal changes for now, and we will refactor all of the names then in a separate PR dedicated to e2e-tests.
So just change the script name for running Stripe e2e tests, and add yours to be similarly named.
"local:e2e:stripe:start": "npm run local:e2e:cleanup-stripe && npm run local:e2e:start-stripe && npm run local:e2e:playwright:ui && npm run local:e2e:cleanup-stripe",
"local:e2e:polar:start": "...",
},There was a problem hiding this comment.
One thing in code part of e2e-tests you would need to change is to make all "Stripe specific" functions be changed with "payment provider abstract" ones.
Now to know which one to run, you could do the environment variable trick in e2e-tests, but we would set them through npm scripts instead.
e.g. "local:e2e:polar:start": "PAYMENT_PROVIDER=polar ..."
Then those abstract functions would use PAYMENT_PROVIDER to know what to do under the hood.
There was a problem hiding this comment.
There is probably a better way to do this (but it would require too much changes).
This will allows us to do this PR reasonably fast and without larger changes, and later on we can improve it.
There was a problem hiding this comment.
OK, understood. So I had already created a separate e2e-tests branch to work on these specifically and I use a PAYMENT_PROVIDER_ID env var to handle provider-specific scripts, as you suggested. But I'll leave this alone until I've completed the requested changes to the application code.
| @@ -79,6 +79,10 @@ app OpenSaaS { | |||
| ] | |||
| }, | |||
|
|
|||
| server: { | |||
| envValidationSchema: import { envValidationSchema } from "@src/server/validation", | |||
There was a problem hiding this comment.
This change is something I think we will want to introduce to open-saas.
It would be nice to introduce env validation.
However, I would also ask you to remove it from the current PR.
You are free to open up a separate PR for this.
| throw new Error(`Unsupported payment processor: ${paymentProcessor.id}`); | ||
| } | ||
|
|
||
| const totalRevenue = await paymentProcessor.getTotalRevenue(); |
There was a problem hiding this comment.
I don't think we want this change.
Payment processor interface shouldn't be concerned with fetching data for your analytics charts.
That is out of scope for that interface.
We want it to be purely focused on supporting the payment part of open-saas.
Additionally, this is also out of scope for this PR.
Not related to adding Polar integration in itself.
I would ask you to revert these changes.
| * }); | ||
| * // Redirect user to session.url for payment | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
This is too verbose.
We purposely use verbose variable names so we don't have to use comments/jsdocs.
Even the start of description Creates a checkout session for payment processing is simply re-telling what its name says createCheckoutSession.
Jsdocs are useful when using the function is tricky and we want to show examples on how to use it properly.
The thing with open-saas is that we already use this function and you can see how we use it.
This is also again out of scope for this PR.
I would only add additional id for Polar.
| * other payment processor code that you're not using from `/src/payment` | ||
| * All available payment processors | ||
| */ | ||
| const paymentProcessorMap: Record<PaymentProcessors, PaymentProcessor> = { |
There was a problem hiding this comment.
Related:
Comment on .env.server.example
https://github.com/wasp-lang/open-saas/pull/486/files#r2266908329
We don't want to tie in the payment processors together.
You only need one.
| } | ||
|
|
||
| console.error('Polar webhook processing error:', error); | ||
| res.status(500).json({ error: 'Webhook processing failed' }); |
There was a problem hiding this comment.
500 means our error, we should be careful here. Is it always our error if it's not verification fault?
| async function handlePolarEvent(event: PolarWebhookPayload, context: any): Promise<boolean> { | ||
| const userDelegate = context.entities.User; | ||
|
|
||
| try { |
There was a problem hiding this comment.
No need to try catch here. We do it above.
| } | ||
|
|
||
| const plan = paymentPlans[planId]; | ||
| if (!plan) { |
There was a problem hiding this comment.
We should fail if we can't extract properly. It's an error state. Not something we should ignore and return 0.
|
|
||
| try { | ||
| switch (event.type) { | ||
| case 'order.created': |
There was a problem hiding this comment.
Why do we handle this, we should only do one-time payments once the the it's paid. We don't need this.
| * @param context Wasp context with database entities | ||
| * @returns Promise resolving to boolean indicating if event was handled | ||
| */ | ||
| async function handlePolarEvent(event: PolarWebhookPayload, context: any): Promise<boolean> { |
There was a problem hiding this comment.
I won't review individual handle event functions now.
I'll do it after the rest of PR is cleaned up.
Thins to make sure before requesting review again:
- Polar works as expected in runtime (e.g. you open a sandbox acocunt and test it with
open-saastemplate, credists, subscriptions, portal url, etc. all should work) - We have a minimum set of webhook events needed
Description
Adds support for Polar payments platform. Closes #441
Related to wasp-lang/wasp#3034 as we need to remove
Stitchesinternally in order to be able to use the correctmoduleResolutionthat Polar's SDK depends on in our tsconfig.Contributor Checklist